Allow setting seed for frequency offsets #881
Conversation
|
|
||
| public FrequencyRandomOffsets(TransitLayer data) { | ||
| public FrequencyRandomOffsets(TransitLayer data, ProfileRequest request) { |
There was a problem hiding this comment.
Is the entire request necessary to be passed as a parameter? I would recommend passing either the seed itself or a MersenneTwister.
There was a problem hiding this comment.
Assuming we want to allow locking the schedules at all I agree: it would be better to pass in only the seed to deter further coupling with the ProfileRequest. The seed should probably include both lat and lon to avoid correlation between all origins in a row across a grid.
I don't see a clear justification for changing the schedules from one origin to another, but locking them at the per-origin (or per-row) level. This would lead to apparently stable differences between neighboring origins that use the same routes to reach most destinations, which are likely to be misinterpreted.
The effects of knowingly deriving results from one single permutation of the schedules are clearer if that permutation is the same for all origins. In that case the seed could just be derived from the TransitLayer (e.g. its center point), and the parameter can just be a boolean for whether to do this.
|
In light of recent discussions summarized at #714 (comment) do we still want to add a |
Addresses #714: with frequency-based routes, users are often distracted by spurious changes in access from random schedule differences between scenarios.
This PR allows "locking" schedules by setting the seed used for randomizing frequency offsets to a deterministic value for each origin (namely, a multiple of
fromLat, as is done in the multi-criteria router: seeFrequencyRandomOffsets). It also adds a field to the analysis request to activate this mode (seeAnalysisRequest). WithlockSchedules: true, a given origin will use the same frequency offsets across scenarios, so resulting changes in access should be systematic ones. This PR also includes other minor changes for related tests and documentation.To test, fetch isochrones repeatedly along an infrequent frequency-based route. Without
lockSchedules, the isochrone should "waver." With this PR andlockSchedules: true(or using buildv6.9-7-ged8378e, which hardcodes the logic to avoid needing to start a backend server via https://github.com/conveyal/r5/tree/lock-lock-schedules), there should be no variability with repeatedly fetched isochrones.In documentation, we should encourage users to run analysis without locking schedules first, to get a sense of variability. If certain corridors show large variability, they may want to use phasing. We've discussed other longer-term changes related to optimized schedules, but the approach proposed here is a minimally invasive one that addresses multiple user requests.